Payment Create - Prevent duplicate payments recorded by concurrent Payment::create calls - #36401
Draft
mattwire wants to merge 1 commit into
Draft
Payment Create - Prevent duplicate payments recorded by concurrent Payment::create calls#36401mattwire wants to merge 1 commit into
mattwire wants to merge 1 commit into
Conversation
…::create calls A payment processor's webhook notification for a charge can race a synchronous front-end/back-office confirmation of the same charge (e.g. paying an existing pending contribution via CRM_Contribute_Form_Contribution_Confirm), both calling Payment::create for the same contribution at once. The existing 'contribution already completed' guard only re-checks status right before completing the order, which isn't atomic, and doesn't stop the FinancialTrxn payment row itself being created twice earlier in the function. Add a per-contribution lock via Civi::lockManager() around create(), and reject a second payment with the same trxn_id for a contribution. Includes a regression test using APIv4 Payment::create/get.
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
PR commands & links...
|
Contributor
|
@mattwire this makes sense - there seems to be a unique index on civicrm_financial_trxn.trxn_id - so that that answers that.... The user experience could be a bit gross if the web hook gets there first as I'm not sure the exception is caught / handled |
Contributor
Author
|
This is the branch that captures on the webhook side - https://lab.civicrm.org/extensions/mjwshared/-/merge_requests/76. I'll look into the user side. |
Contributor
Author
|
@eileenmcnaughton I've just posted an alternative here - #36426 - feels a bit cleaner? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
A payment processor's webhook notification for a charge can race a synchronous front-end/back-office confirmation of the same charge (e.g. paying an existing pending contribution via CRM_Contribute_Form_Contribution_Confirm), both calling Payment::create for the same contribution at once.
The existing 'contribution already completed' guard only re-checks status right before completing the order, which isn't atomic, and doesn't stop the FinancialTrxn payment row itself being created twice earlier in the function.
Add a per-contribution lock via Civi::lockManager() around create(), and reject a second payment with the same trxn_id for a contribution. Includes a regression test using APIv4 Payment::create/get.
Found with stripe extension on a client site (webhook for payment completion comes in quicker than contribution page confirm processes and they sometimes end up concurrently recording a payment).
Before
No lock.
After
Lock around payment::create
Technical Details
Comments
Are there any situations where we would legitimately have a second payment with the same trxn_id?